Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed #1

Open
wants to merge 1 commit into
base: 0_dev_training
Choose a base branch
from
Open

fixed #1

wants to merge 1 commit into from

Conversation

alina42
Copy link

@alina42 alina42 commented Jun 7, 2021

test

jdavcs pushed a commit that referenced this pull request Sep 21, 2021
jdavcs pushed a commit that referenced this pull request Sep 21, 2021
jdavcs added a commit that referenced this pull request Feb 8, 2024
Reason: late evaluation of arguments causes errors in mypy;
also this seems to be the more common (as per SA docs) and more readable
method compared to lambdas.

Downside #1: a typo in the string value won't be caught by lint tools.
Downside galaxyproject#2: if there's a sufficiently complex statement, it will be
hard to read if it's all on one line;

primaryjoin = "and_(HistoryDatasetAssociation.history_id == History.id, not_(HistoryDatasetAssociation.deleted), HistoryDatasetAssociation.visible)"

splitting across lines will require surrounding each line with quotes:

primaryjoin = (
   "and_("
   "HistoryDatasetAssociation.history_id == History.id,"
   "not_(HistoryDatasetAssociation.deleted),"
   "HistoryDatasetAssociation.visible,"
    ")"
)

Still, I think it's better than the alternative (lambda + multiple type-ignores
after each line:

primaryjoin = lambda(
   and_(
        HistoryDatasetAssociation.history_id == History.id,  # type:ignore[has-type, arg-type]
        not_(HistoryDatasetAssociation.deleted),  # type:ignore[has-type, arg-type]
        HistoryDatasetAssociation.visible,  # type:ignore[has-type, arg-type]
    )
)
jdavcs pushed a commit that referenced this pull request Mar 18, 2024
* Rename `delta` to `eps` for float-based assertions

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants